-
Notifications
You must be signed in to change notification settings - Fork 343
Introduce SpriteSequence, a covariant supertype of SpriteList. #2647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4890b9f
to
b11fb9a
Compare
@DragonMoffon and I want to change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for starting taking a crack at this. I appreciate it since it's sorely needed (see #2580 and similar 😅).
For the moment, I'm a little confused on some abc
-related details as in this comment. If nobody beats me to giving this a closer look, I may need a few days before I can give it the full attention it deserves.
@eruvanos btw, do you think we should add a typing-specific tests folder or similar as an exception to the tests folder being ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I addressed the comments so far.
I'm not sure what your squashing policy is. For now I added a comment addressing the review, but I'm also happy to squash it with the parent commit.
@pushfoo May I suggest a ping here? Is there anything else you'd like me to do? Thank you. |
@pushfoo Regarding the typed test folder: if we have tests that benefit from such a setup I am fine, I wonder if it makes our test structure more complicated. Regarding Scene and TileMap: we can iterate on them after we merge this PR. I just created a small code base over easter which uses Scene and TileMap, so I could doublecheck the improved versions. @sjrd we do not have a strict squashing policy, I prefer squashed PRs to reduce the amount of commits we have to read through during write up of release notes. |
As we are just out of a while loop whose stopping condition is that `sprite_lists` is empty, calling `clear()` on it is redundant.
There was already a method `register_sprite_list` to handle all additions to `sprite_lists`. We add a corresponding method `_unregister_sprite_list` to handle removals. We make the typings of these methods stricter, so that we can enforce the correct typing invariant on `sprite_lists`. We also make that invariant clearer in a comment. `sprite_lists` is unfortunately unsafely visible to everyone. So a user of the class could still violate the invariants. At least now the *intended* usage is safe.
This is similar to the fix done to `check_for_collision_with_list` done in c387717.
Adding type parameters to some `SpriteList`s. One allows to get rid of a cast.
Thanks. I squashed the last two commits, and added the entry to the changelog. I had to rebase on top of #2652 not to have conflicts. The only changes besides rebasing are the |
This is done by analogy to `collections.abc.Sequence`, which is a covariant supertype of `list`. Before this commit, many parts of the codebase used `SpriteList`s without type arguments (defaulting to `Unknown`). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing. Using `SpriteSequence`, we can add correct type arguments to almost all of the references that were using `SpriteList`s before. The only missing pieces are `Scene` and `TileMap`. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of their `SpriteList`s. We cannot make it sound without breaking their APIs, so we do not change them. As a bonus, we can now create lists of `SpriteList`s with varying type arguments, and generically call `draw` or `update` on them. Previously, the only common supertype of `SpriteList[A]` and `SpriteList[B]` was `object`, which meant it was not possible to call those methods on them. In a sense, that ability mostly subsumes the convenience provided by `Scene`. A `list[SpriteSequence[BasicSprite]]` is almost as convenient, while being type-safe.
@pushfoo are you fine with the changes? I think we could move on with this and iterate on Scene and Tilemap (and typed tests afterwards). What is your opinion? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. I'll say let's merge it to keep velocity and revisit if we need to. Ty for your contribution and your patience.
This is done by analogy to
collections.abc.Sequence
, which is a covariant supertype oflist
.Before this commit, many parts of the codebase used
SpriteList
s without type arguments (defaulting toUnknown
). That was the only way to allow reasonable usages of the given methods and attributes. However, doing so results in weaker typing.Using
SpriteSequence
, we can add correct type arguments to almost all of the references that were usingSpriteList
s before.The only missing pieces are
Scene
andTileMap
. Unfortunately, their APIs are fundamentally unsound wrt. the type arguments of theirSpriteList
s. We cannot make it sound without breaking their APIs, so we do not change them.As a bonus, we can now create lists of
SpriteList
s with varying type arguments, and generically calldraw
orupdate
on them. Previously, the only common supertype ofSpriteList[A]
andSpriteList[B]
wasobject
, which meant it was not possible to call those methods on them.In a sense, that ability mostly subsumes the convenience provided by
Scene
. Alist[SpriteSequence[BasicSprite]]
is almost as convenient, while being type-safe.This was prompted by seeing students struggle with the absence of a common supertype for
SpriteList[A]
andSpriteList[B]
. Students resorted tocast
s orAny
s (or smuggled their way out of that usingScene
) to work around the issue. WithSpriteSequence
, this shouldn't be an issue anymore.It turns out that the introduction of
SpriteSequence
allows to more safely type many APIs of Arcade, so hopefully this addition carries its weight.